Skip to content

Fix Onnx 23 Rotary Fusion #2576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Sep 26, 2025
Merged

Fix Onnx 23 Rotary Fusion #2576

merged 5 commits into from
Sep 26, 2025

Conversation

gramalingam
Copy link
Collaborator

Fix Onnx 23 Rotary Fusion

@gramalingam
Fix cache double size in rotary embedding
a770d98 
Signed-off-by: Ganesan Ramalingam <[email protected]>
@gramalingam
Minor fix and test case
0b9afd9 
Signed-off-by: Ganesan Ramalingam <[email protected]>
@gramalingam gramalingam marked this pull request as draft September 25, 2025 17:03
@codecov Codecov
Copy link

codecov bot commented Sep 25, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 84.90566% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.04%. Comparing base (168fd8a) to head (a6fd40c).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...xscript/rewriter/rules/fusion/_rotary_embedding.py 78.94% 2 Missing and 2 partials ⚠️
...pt/rewriter/rules/fusion/_rotary_embedding_test.py 86.66% 2 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2576      +/-   ##
==========================================
- Coverage   70.10%   70.04%   -0.06%     
==========================================
  Files         222      223       +1     
  Lines       26184    26215      +31     
  Branches     2581     2583       +2     
==========================================
+ Hits        18355    18363       +8     
- Misses       6929     6946      +17     
- Partials      900      906       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 25, 2025
View reviewed changes
@gramalingam
Move tests
48c3cc9 
Signed-off-by: Ganesan Ramalingam <[email protected]>
@gramalingam gramalingam changed the title DRAFT: Fix Onnx 23 Rotary Fusion Fix Onnx 23 Rotary Fusion Sep 25, 2025
@gramalingam gramalingam marked this pull request as ready for review September 25, 2025 20:02
@justinchuby
Copy link
Collaborator

justinchuby commented Sep 25, 2025 •
edited
Loading

Nice: looks like the values now match

(tensor([[[-1.0310,  0.0658, -0.1172,  ...,  0.2810, -0.0586, -0.4056],
         [ 1.5216, -2.2804,  2.4418,  ...,  0.6617, -2.2747,  1.3125],
         [ 0.3216, -0.1102, -0.7690,  ..., -2.1603, -2.1465, -0.0737]],

        [[-0.5299,  0.3161, -0.0311,  ...,  0.5096,  1.7270,  0.2317],
         [-0.0963,  0.4574, -1.3810,  ..., -1.6085,  3.8896,  1.3088],
         [ 0.5815, -2.3405,  0.8283,  ...,  1.8330, -0.1297,  0.9608]]]),)
------------
Gemma3ModelOutputWithPast(last_hidden_state=
tensor([[[-1.0310,  0.0658, -0.1172,  ...,  0.2810, -0.0586, -0.4056],
         [ 1.5215, -2.2804,  2.4418,  ...,  0.6617, -2.2747,  1.3125],
         [ 0.3216, -0.1102, -0.7690,  ..., -2.1603, -2.1465, -0.0737]],

        [[-0.5299,  0.3161, -0.0311,  ...,  0.5096,  1.7270,  0.2317],
         [-0.0967,  0.4576, -1.3808,  ..., -1.6082,  3.8898,  1.3089],
         [ 0.5815, -2.3406,  0.8283,  ...,  1.8331, -0.1296,  0.9608]]],
       grad_fn=<MulBackward0>), past_key_values=None, hidden_states=None, attentions=None, image_hidden_states=None)

@justinchuby
Copy link
Collaborator

Hmm. It doesn't seem right: There is no rotary nodes in the graph
image

@gramalingam
Change const check
6864ee0 
Signed-off-by: Ganesan Ramalingam <[email protected]>
@gramalingam
Conditionally execute test
a6fd40c 
Signed-off-by: Ganesan Ramalingam <[email protected]>
@gramalingam gramalingam enabled auto-merge (squash) September 26, 2025 18:31
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 26, 2025
View reviewed changes
onnxscript/rewriter/rules/fusion/_rotary_embedding_test.py
onnx_version = Version(onnx.__version__)
min_version = Version("1.19.1")
is_stable = not (onnx_version.is_devrelease or onnx_version.is_prerelease)
if onnx_version >= min_version and is_stable:

Check failure

Code scanning / CodeQL

Potentially uninitialized local variable Error

Local variable 'is_stable' may be used before it is initialized.

Copilot Autofix

AI 24 days ago

Copilot could not generate an autofix suggestion

Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.

@gramalingam gramalingam merged commit dddf0c2 into main Sep 26, 2025
31 of 32 checks passed
@gramalingam gramalingam deleted the rama/rotary branch September 26, 2025 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinchuby justinchuby justinchuby approved these changes

Assignees

No one assigned

Labels

module: rewriter

Projects

ONNX Script Review Board
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gramalingam @justinchuby